fix: correct TIME_WINDOW slot layout and prune unused aggregates#25798
fix: correct TIME_WINDOW slot layout and prune unused aggregates#25798ck89119 wants to merge 12 commits into
Conversation
TIME_WINDOW derived its output batch layout in two independent places: the
planner compacted ProjectList slots with a counter that only advanced for
projected entries, while constructTimeWindow/calRes built the batch from the
unpruned AggList. Whenever an outer query discarded any window output the two
drifted apart and the projection read a neighbouring column.
That is a silent wrong-answer bug, not only a missed optimization:
select c from (select _wstart a, max(v) b, min(v) c from t
interval(ts, 5, second)) x
returned max(v) under the min(v) alias. Discarding _wstart instead produced
"unsupported type INT for MYSQL_TYPE_TIMESTAMP"; discarding _wstart while
keeping _wend failed remapping outright, because the wstart branch used `break`
inside a switch and skipped the wend branch with it; and fill(prev) panicked on
an out-of-range index, since constructFill sized ColLen from an AggList that
was never pruned alongside the window's.
Introduce BuildTimeWindowLayout as the single source of truth for the layout
and have both the planner and the compiler consume it. Pruning then follows:
only retained aggregates take a reference on their inputs, so the child AGG
drops those inputs too, and FILL prunes its columns in step with the window.
A row carrier is kept when every value is discarded, because calRes sizes its
batch off Vecs[0] and the window count is still the row count.
GROUP BY alongside a time window previously panicked. The window was never
partitioned at all: the child AGG grouped by (key, trunc_ts) but the window
sorted by trunc_ts alone, interleaving groups into one global stream, and the
operator never emitted the group column. Carry those keys as
TimeWindowPartitionBy, order by them ahead of the timestamp, and restart the
window state at each boundary so every group gets its own windows.
Two further latent bugs surfaced and are fixed here:
- node.WEnd's column reference was never remapped. The operator evaluates it
against a batch holding only the timestamp, so it worked only while the
window key happened to be group column 0.
- every flush after the first orphaned its aggregate vectors. Previously this
needed more than 8192 windows; partitioning makes flushing routine.
Verified against a per-partition reference over 9200 rows and 4 partitions:
identical results across tumbling, sliding, and fill(prev) specs, including
8794 rows spanning batch boundaries.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # pkg/pb/plan/plan.pb.go
Review findings on the partitioned time window, verified and fixed: - fill(prev/next/linear) carried values across partition boundaries: prev filled a group's leading NULL with the previous group's value, and next dropped whole batches. Partition key positions now travel to the FILL operator (Node.time_window_partition_col_pos, resolved by forcing the child window to project its keys), and all three modes treat a key change as a hard boundary. NEXT and LINEAR are rewritten to materialize the input first: their per-column streaming state machine returned early whenever a batch contained no NULL, silently discarding every batch after it. - fillRows reset withoutFill per batch, so a window whose rows arrived in one batch and whose closing row arrived in the next was emitted as empty and dropped. The flag now resets only where windows actually switch. - remakeAggs overwrote aggregate executors without freeing them, leaking one full set of aggregate state per partition. - FILL pruning discarded side-effecting fill values (fill(value, sleep(1))) together with their unprojected columns; they now pin the matching window slot, mirroring how projection pruning treats side effects. - TimeWindowPartitionBy joins DeepCopyNode (whose TIME_WINDOW/FILL fields were never copied at all), the plan visitor, and replaceColumnsForNode. - constructTimeWindow preallocates its slices (make static-check). - The regenerated golden file had rewritten every pre-existing expectation into the new recorder format; the old lines are restored and only this PR's cases are appended. Partitioned fill(prev/next/linear) verified against per-partition references over 9200 rows / 4 partitions: identical. New unit tests cover partition boundaries inside and across batches for both operators, batch-split invariance, and the null-free-batch case that the old state machine dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking: the new partitioned sliding TIME_WINDOW path is not reusable after Reset.
I reproduced this at the current head (0949294) by running a partitioned sliding TimeWin to ExecStop, calling Reset + Prepare, attaching a fresh child with the same rows, and running it again. The second execution reliably panics in aggState.grow from container.nextWindow -> AggFuncExec.GroupGrow.
resetParam only resets status/end/group/window slices and three partition fields. It leaves generation-local state such as last, i, the current/previous/break cursors, partEnd, and the flushed aggregate executors intact. After the first execution, receive therefore resumes through nextWindow with an aggregate whose flushed result vector is no longer usable, causing the nil dereference. Merely clearing one cursor would still leave stale rows/state or unbounded retained buffers across reuse.
Please make Reset establish a complete generation boundary: reset every control/cursor flag, reset or recreate aggregate state after Flush, and make buffered vector reuse start at index 0. Add a regression test that (1) exhausts the partitioned sliding operator, (2) calls Reset and Prepare, (3) exhausts it again with fresh input, (4) compares both complete outputs, and (5) verifies the mpool returns to zero after Free. The current TestTimeWin calls Exec only once on the interval path, so it cannot cover this failure.
I also checked planner/compiler layout alignment, partition-boundary NULL semantics, cross-batch PREV/NEXT/LINEAR behavior, ownership cleanup, and the targeted package tests. The existing tests pass, but the lifecycle reproducer fails with SIGSEGV, so this needs to be fixed before merge.
A partitioned sliding TIME_WINDOW run to ExecStop could not be reused: Reset left the buffer cursor, window cursors/bounds, last/partEnd flags and the flushed aggregate executors intact, so the second execution resumed through nextWindow and crashed in AggFuncExec.GroupGrow. - resetParam now rewinds every per-generation field, so buffered vector reuse restarts at index 0 and receive routes into firstWindow. - Reset releases the previous generation's flushed batch (the aggregate prefix it owns) and discards the aggregate executors; Prepare rebuilds them behind a gate separate from the expression executors. - freeVector also releases the _wstart/_wend staging vectors. - Regression tests exhaust the operator, Reset+Prepare, run it again on fresh input, compare both outputs and assert the mpool returns to zero, for the partitioned sliding, plain sliding and interval paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KRbU3MwwDsG2RNXYvQ2rpX
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking: the new bounded-memory spill path still loses the streaming and early-stop guarantee after the threshold is crossed.
At head 6123607, driveFill enters collectSpill whenever the spill is not ready, and collectSpill can only return after the child reaches EOF. It does not stop when a NEXT endpoint, LINEAR right endpoint, or partition boundary has already resolved the earliest spilled suffix.
I reproduced this deterministically with one-row NEXT batches:
10, NULL, NULL, 40, 50, 60
and SpillThreshold = 2 rows. The first Fill.Call returns 10 after one child pull. During the second Fill.Call, row 40 closes the pending NULL gap on the fourth child pull, so the next output is fully determined and an outer LIMIT could stop there. The implementation instead performs seven child pulls, consuming 50, 60, and EOF before returning. The unrelated tail can be arbitrarily large, so this creates unbounded extra scan work, spill I/O, and first-result latency after a large gap. LINEAR follows the same EOF-only collection path.
Please make spill replay segment-based: finalize and replay the earliest segment once every unresolved column affecting that segment has reached its endpoint, partition boundary, or EOF, then resume the child after replay. Add deterministic child-call-count regressions for NEXT and LINEAR, including multiple fill columns and a partition boundary.
The correctness, Reset/reuse, T_any partition handling, pruned-boundary cleanup, and resource ownership checks otherwise look closed on this head. Local modified-package tests passed; fill/timewin race tests passed with count 10; and a temporary merge with current main passed the modified-package tests.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No blocking correctness, compatibility, resource-lifecycle, concurrency, or pruning regressions found in the reviewed diff.
aunjgr
left a comment
There was a problem hiding this comment.
[P1] The new closed-segment replay fixes the single-column case, but scanSegment still requires all segmentWait entries to be false at the same row. With two fill columns and future rows alternating [value,NULL], [NULL,value], every earlier prefix becomes fully resolved on the next row, yet one newest run is always pending. allResolved never succeeds, so an unbounded child is still drained forever without emitting that safe prefix. Track the earliest unresolved row per column and replay up to the minimum safe watermark instead of waiting for simultaneous resolution. Add an alternating multi-column streaming regression that asserts child-call count before EOF.
aunjgr
left a comment
There was a problem hiding this comment.
[P1] The new closed-segment replay fixes the single-column case, but scanSegment still requires all segmentWait entries to be false at the same row. With two fill columns and future rows alternating [value,NULL], [NULL,value], every earlier prefix becomes fully resolved on the next row, yet one newest run is always pending. allResolved never succeeds, so an unbounded child is still drained forever without emitting that safe prefix. Track the earliest unresolved row per column and replay up to the minimum safe watermark instead of waiting for simultaneous resolution. Add an alternating multi-column streaming regression that asserts child-call count before EOF.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No correctness, compatibility, lifecycle, resource, concurrency, performance, or test-coverage regressions found. PR head matches the checkout; required CI checks are successful. Local tests could not run because go is unavailable in this environment.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No blocking correctness, compatibility, lifecycle, concurrency, or resource-handling regressions found in origin/main...0e9b18d. Focused package tests could not run because this worktree lacks cgo/libmo.dylib.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking correctness issue remains in the LINEAR spill lifecycle. I rechecked the full diff and all previous blockers on the current head: Reset/reuse, incremental streaming, GROUP BY NULL/T_any, pruned boundary cleanup, early spill replay, and the multi-column safe watermark are closed. However, a consecutive spill generation can discard the left endpoint of an already-resolved LINEAR run and turn the interpolated row back into NULL.
Current-head evidence: the official fill/timewin/plan/compile tests, race tests, build, vet, and CI are green, but a deterministic differential test fails with the three-row reproducer in the inline comment. Please preserve the segment-entry left endpoint across the next spill (including after consumeLinear has consumed/cleared linSeed) and add regressions for consecutive spill generations with one/multiple columns and partition boundaries.
| } | ||
| spill.updateSafeWatermark() | ||
| if ap.FillType == plan.Node_LINEAR && len(ctr.linSeed) > 0 { | ||
| spill.linearLeft = ctr.linSeed |
There was a problem hiding this comment.
[P1] Preserve the LINEAR segment-entry seed when an already-resolved batch spills again. After a prior spill replays, ctr.linSeed is the left endpoint. consumeLinear uses it to interpolate a later run, then clears it when the right endpoint arrives (fill.go:457-470). If that resolved batch crosses the threshold, this block transfers the now-cleared seed into spill.linearLeft; reverse replay still sees the original-NULL markers, and finishLinearBatch has no valid left endpoint, so it re-adds NULL.
Minimal reproducer on c7f6568a58, one partition, DECIMAL128, SpillThreshold=1:
batch 1: (10, 100)
batch 2: (NULL, NULL), (20, 200)
expected: (10,100), (15,150), (20,200)
actual: (10,100), (NULL,NULL), (20,200)
The small threshold only makes the production state transition deterministic; the same loss occurs whenever consecutive segments cross the byte threshold. Keep the left endpoint associated with the beginning of every spilled suffix/segment even after its in-memory run has resolved, and cover consecutive spill rounds (plus multi-column/partition cases).
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No actionable findings. Historical review concerns are addressed in the current head; the slot layout, pruning, partitioning, fill/spill lifecycle, and reset/resource paths are internally consistent. Current PR CI is green.
What type of PR is this?
Which issue(s) this PR fixes:
issue #25699
Part of #25612. Follows #25613, which deferred
TIME_WINDOWbecause itsrow-count and special-slot contracts needed an execution-layer design.
What this PR does / why we need it:
The issue describes a missed optimization; it is also a wrong-answer bug
TIME_WINDOW's output batch layout was derived independently in two places:remapAllColRefsaddressed slots with a counter that only advanced forprojected entries;
constructTimeWindow/calResbuilt the batch from the unprunedAggList.As soon as an outer query discarded any window output, the two drifted and the
projection read a neighbouring column. Reproduced on a live instance
(
interval(ts, 5, second)over two windows; baseline_wstart, max(v), min(v)=
(00:00:00, 20, 10),(00:00:05, 40, 30)):select c from (select _wstart a, max(v) b, min(v) c from t interval(ts,5,second)) x20, 40— silently returnsmax(v)10, 30select a from (…same…) xERROR 1105: unsupported type INT for MYSQL_TYPE_TIMESTAMPselect d from (select _wstart a, _wend d, max(v) b …) xERROR 1064: Column remapping failedselect c from (… fill(prev)) xpanic: index out of range [1] with length 110, 30Sliding windows are affected identically (
select creturned40, 40formin(v)). Causes, respectively: the compacted counter; the same counterapplied to a boundary slot; a
breakinside theswitchthat skipped the_wendbranch along with_wstart; andconstructFillsizingColLenfrom aFILL.AggListthat was never pruned alongside the window's.Fix
BuildTimeWindowLayoutbecomes the single source of truth for the layout(
[aggregates…, _wstart?, _wend?, partition keys…]), consumed by both theplanner and the compiler, so the two cannot drift again. Pruning then follows
from it:
AGGdrops those inputs too — the "prune aggregates and their inputs" the issue
asks for;
FILLprunes its columns andFillValin step with the window. This must bedecided before
FillVal's refcounts, becausefill(linear)buildsFillValout of references to those very aggregates and would otherwise keepevery one alive;
calRessizes itsbatch off
Vecs[0], and the window count is still the row count. A boundaryis preferred, since it runs no aggregate;
_wstartreference maps to the one boundary slot, so repeats cannotdrop a projection.
select c from (… max(v), min(v) …) xnow compactsAggListfrom 3 entries to1, and the child
AGGdropsmax(v)and its input column.GROUP BY + time window: partitioned execution
select _wstart, other, max(v) from t group by other interval(ts,5,second)previously panicked. The window was never partitioned at all — the child
AGGgrouped by
(other, trunc_ts)while the window sorted bytrunc_tsalone,interleaving groups into one global stream, and the operator never emitted the
group column. This PR carries those keys as
TimeWindowPartitionBy, orders bythem ahead of the timestamp, and restarts the window state at each boundary so
each group gets its own windows. Partition keys are never pruned (dropping one
would merge groups) and land in the tail slots, keeping the aggregates a prefix
for
FILL.Two further latent bugs found and fixed here
node.WEnd's column reference was never remapped. The operator evaluatesit against a batch holding only the timestamp, so it worked only while the
window key happened to be group column 0;
group by idmoves it to 1 and itreads out of range.
resetTimeWindowTsColRefmakes the contract explicit.Flush()results are owned by that batch (boundaries belong to theirexpression executors, partition keys to
ctr.partOut). Previously this neededmore than 8192 windows; partitioning makes flushing routine.
Also removes
fill.AggIds, which was assigned byconstructFilland never read.Review round: partition-aware FILL and further fixes
All seven review findings were verified and fixed:
group's leading NULL with the previous group's value; next dropped whole
batches. The partition key positions now travel to the FILL operator
(
time_window_partition_col_pos), and all three modes treat a key change asa hard boundary. NEXT/LINEAR are rewritten to materialize their input first:
the old per-column streaming state machine returned early whenever a batch
contained no NULL and silently discarded every batch after it (a
pre-existing bug that partitioning made routine).
fillRowsresetwithoutFillper batch instead of per window, so a window whose rows camein one batch and whose closing row came in the next was emitted as empty.
Found by the new batch-split invariance test.
executor still owns its state after
Flush; it is now freed first.fill(value, sleep(1))on adiscarded column now pins the matching window slot (verified end-to-end:
1.02s with sleep retained vs 0.017s with a plain value pruned).
TimeWindowPartitionByjoins the planner infrastructure —DeepCopyNode(whose TIME_WINDOW/FILL fields were never copied at all),the plan visitor, and
replaceColumnsForNode, each with tests.make static-checkpasses — the twopreallocfindings are fixed.genrshadrewritten them into the new recorder format; only this PR's cases are
appended now (verified: zero deleted lines, suite 61/61).
Partitioned
fill(prev/next/linear)was re-verified against per-partitionreferences over 9200 rows / 4 partitions: identical (576/576/576/863 rows
across the four specs).
Tests
gaps, comparing
group by p <spec>against per-partitionwhere p=N <spec>.Identical across
interval(1 minute)(1723 rows),interval(3 minute) fill(prev)(576),sliding(2 minute)(863),sliding(20 minute)(88), andinterval(30s) sliding(10s)(8794 rows, spanning batch boundaries). Thiscaught two bugs in the implementation: partitions losing their trailing
windows, and an out-of-range read that broke all non-sliding time-window
queries.
BuildTimeWindowLayout(slot/boundary/partition/repeat/empty cases anda planner-vs-compiler consistency check); plan-shape tests for each pruning
shape above; operator tests for partition reset, trailing windows,
single-partition equivalence with the unpartitioned operator, NULL keys
grouping together, and the pass-through path — all asserting
CurrNB() == 0,which is what surfaced the flush leak.
optimizer/column_pruningcovering every query in thetable above plus sliding, all three
fillmodes, empty input, row carriers,and partitioned windows.
Coverage of changed code:
time_window_layout.go100%; theTIME_WINDOW/FILLremap branches 92.3%;
colexec/timewin35.4% → 72.9%.Suites:
time_window31/31,optimizer1078/1078,window1543/1559 — the 4failures are
external01external-table reads returning 0 rows, verified tofail identically on an unmodified baseline build (
select count(*) from external01has noTIME_WINDOWnode at all).Review round 2: TimeWin reusable after Reset (195c1a0)
Reproduced the reported crash: a partitioned sliding TimeWin run to
ExecStop,then
Reset+Preparewith a fresh child, panicked innextWindow -> AggFuncExec.GroupGrowon the second run.resetParamnow rewinds every per-generation field: the buffer cursori(sotsVec/aggVec/partVecreuse restarts at index 0 instead ofappending after stale entries), the window cursors and bounds
(
curVecIdx/curRowIdx,preVecIdx/preRowIdx,left/right,nextLeft/nextRight),last/lastVal,withoutFill, and all partitionbreak state (
partEnd,breakVecIdx/breakRowIdx,partLast*). With thecursors at zero,
receiveroutes intofirstWindowagain.Resetnow establishes the generation boundary for owned memory: it freesthe aggregate prefix of the last flushed batch (the only part that batch
owns — boundaries belong to their expression executors, partition keys to
partOut), drops the batch reference, and discards the flushed aggregateexecutors (they cannot be rewound after
Flush, the same propertyremakeAggsdocuments).Preparerebuilds the aggregates behind a gateseparate from the expression executors.
freeVectoradditionally releases the_wstart/_wendstaging vectors,which the
Resetpath would otherwise retain.TestTimeWinReuseAfterReset,TestTimeWinIntervalPathReuseAfterReset): exhaust the operator,Reset+Prepare, exhaust it again on fresh input, compare the twocomplete outputs, and assert
CurrNB() == 0afterFree— for thepartitioned sliding, plain sliding, and interval pass-through paths. The
first generation reads two child batches and the second reads one, so the
index-0 buffer-reuse claim is exercised, not just asserted. Reverting the
fix makes the new test fail with the exact reported stack (panic in
nextWindow).Verification:
colexec/timewinUT green incl.-race, coverage 77.5%(
Reset/resetParam100%); BVTtime_window31/31 andoptimizer/column_pruning61/61; prepared-statement re-execution of apartitioned sliding window returns identical results across executions.
Review round 3: fill(next/linear) incremental again, not fully materialized (957441a)
The round-2 rewrite gathered the entire child stream before emitting the first
NEXT/LINEAR result, which turned even the no-NULL path into O(total rows)
retained memory and full-input first-row latency (an outer
LIMIT 1could notstop the child; a wide time range could OOM although no look-ahead was needed).
Replaced with a batch-level incremental engine:
batsis now a pending FIFO keyed by an absolute sequence number (baseSeq),so a captured coordinate stays valid after the FIFO pops its head. A batch is
emitted as soon as every fill column has resolved its rows; the child is
pulled again only when nothing is emittable.
toFreereleases the batch handedto the parent on the previous
Call.non-NULL value; LINEAR keeps the last non-NULL as a left endpoint and pins its
batch until superseded, interpolating a pending run against it. Partition
boundaries drop the pending candidates (they stay NULL), preserving the
existing cross-partition semantics. A no-NULL NEXT stream buffers nothing; a
no-NULL LINEAR stream buffers at most one batch. A genuinely long unresolved
suffix still buffers until the next value, which is inherent to the operator's
semantics (the original streamed the same way); no disk spill.
batch after exactly one child pull and LINEAR after one look-ahead, both well
before child EOF, with
len(ctr.bats) <= 1throughout; plus long cross-batchgaps, EOF tails, independent multi-column gaps, and cross-batch LINEAR
interpolation.
Verification:
colexec/fillUT green incl.-race, coverage 85.4%,golangci-lint 0 issues; BVT
time_window31/31 andoptimizer/column_pruning61/61; partitioned prev/next/linear/value verified end to end with no
cross-partition bleed and
LIMIT 1returning immediately.